MVJ-112: area search attachments: hide user and attachment path from API response in public #819

NC-jsAhonen · 2025-01-24T12:14:44Z

With these changes, the API response to the POST requests will hide sensitive information about the attachments.

henrinie-nc

How about forms attachment?

plotsearch/serializers/plot_search.py

NC-jsAhonen · 2025-01-27T09:35:15Z

How about forms attachment?

Now added.

henrinie-nc · 2025-01-27T09:46:41Z

Looks good! It could be useful to add a comment, or add the list of fields in a named variable to indicate what is being done and why

henrinie-nc

Changed the approve to request changes, realized that this should also include tests.

NC-jsAhonen · 2025-01-28T14:33:36Z

Created variables for the excluded fields and commented. They turned out to be useful in the unit tests as well.

henrinie-nc · 2025-01-29T11:33:44Z

forms/tests/test_api.py

+
+
+@pytest.mark.django_db
+def test_area_search_attachment_post_public(


Should this test exist in the areasearch app?

forms/tests/test_api.py

henrinie-nc · 2025-01-29T11:41:08Z

forms/tests/test_api.py

@@ -430,6 +432,97 @@ def test_attachment_delete(
    assert os.path.isfile(file_path) is False


+@pytest.mark.django_db
+def test_attachment_post_public(


The test name could clarify what is it testing for, and enhanced with comments. It is not very clear what is being tested, the name indicates it is just to test that the endpoint works

henrinie-nc · 2025-01-29T11:42:11Z

forms/tests/test_api.py

+        )
+        == 0
+    )
+


How about testing the areasearch endpoint, that it doesnt return the undesired fields?

forms/tests/test_api.py

henrinie-nc · 2025-02-03T11:04:21Z

forms/tests/test_api.py

+    response = admin_client.post(url, data=payload, content_type="application/json")
+
+    # When an answer is created, the HTTP response should not contain attachments
+    assert response.json().get("attachments") is None


It could also be checked that the key does not exist, this makes it seem a little bit that test expects the key to be there and value to be None

forms/tests/test_api.py

henrinie-nc · 2025-02-03T11:22:53Z

utils/viewsets/mixins.py

@@ -165,3 +165,9 @@ def update(self, request, *args, **kwargs):
    @action(methods=["get"], detail=True)
    def download(self, request, pk=None, file_field: str | None = None):
        return FileDownloadMixin.download(self, request, pk, file_field=file_field)
+
+
+class DisablePublicDownloadMixin:


This seems like a good addition! What do you think about the following approach: Implement two versions of the FileMixin, one that allows only upload, and one that allows upload and download? There is potential that having this disable makes it a bit messy regarding the mixins. If there were two options to choose from, it might be easier to realize that there is this difference

henrinie-nc · 2025-02-03T11:26:53Z

Would it make sense to also test the serializers? Hopefully that would be rather simple, but would confirm they work as intended down the line.

plotsearch/tests/serializers/test_plot_search_serializers.py

henrinie-nc · 2025-02-05T07:51:02Z

plotsearch/tests/api/test_plot_search.py

+    assert os.path.isfile(file_path) is True
+    response = admin_client.delete(url)
+
+    assert response.status_code == 405


The 405 could come from from rest_framework import status, status.HTTP_405_METHOD_NOT_ALLOWED

@henrinie-nc That is a good and refined approach because it makes sure that the status code is exactly the same what Django Rest Framework uses, but I'm not sure if it's an improvement here over a plain understandable status code number.

Indeed it doesnt add anything to the comparison, but I think it helps humans reading it to realize what is expected

juho-kettunen-nc

My change requests are about test understandability. I didn't find holes in the implementation when I tested it locally. Good work! Some of Henri's comments should be addressed or replied to.

plotsearch/tests/api/test_plot_search.py

juho-kettunen-nc · 2025-02-05T13:37:28Z

plotsearch/tests/api/test_plot_search.py

+    assert os.path.isfile(file_path) is True
+    response = admin_client.delete(url)
+
+    assert response.status_code == 405


@henrinie-nc That is a good and refined approach because it makes sure that the status code is exactly the same what Django Rest Framework uses, but I'm not sure if it's an improvement here over a plain understandable status code number.

plotsearch/tests/serializers/test_plot_search_serializers.py

juho-kettunen-nc

My grievances have been addressed. We will fix merge conflicts later today, because my yesterday's merge touched the same files.

… responses

test_attachment_upload: remove docstring test forms attachment upload and download: rename functions and improve comments excluded attachment fields: improve commenting use pytest raises improve assert test serializers

NC-jsAhonen requested review from henrinie-nc and juho-kettunen-nc January 24, 2025 12:14

henrinie-nc requested changes Jan 24, 2025

View reviewed changes

henrinie-nc reviewed Jan 24, 2025

View reviewed changes

plotsearch/serializers/plot_search.py Outdated Show resolved Hide resolved

NC-jsAhonen force-pushed the MVJ-112-api-does-not-give-file-url-in-response branch 2 times, most recently from 7cde77f to 9863eb9 Compare January 27, 2025 09:26

NC-jsAhonen requested a review from henrinie-nc January 27, 2025 09:33

henrinie-nc approved these changes Jan 27, 2025

View reviewed changes

henrinie-nc requested changes Jan 27, 2025

View reviewed changes

NC-jsAhonen requested a review from henrinie-nc January 28, 2025 14:33

henrinie-nc reviewed Jan 29, 2025

View reviewed changes

forms/tests/test_api.py Outdated Show resolved Hide resolved

henrinie-nc reviewed Jan 29, 2025

View reviewed changes

forms/tests/test_api.py Show resolved Hide resolved

henrinie-nc reviewed Jan 29, 2025

View reviewed changes

forms/tests/test_api.py

)

== 0

)

Copy link

Contributor

henrinie-nc Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about testing the areasearch endpoint, that it doesnt return the undesired fields?

NC-jsAhonen requested a review from henrinie-nc January 30, 2025 11:15

henrinie-nc reviewed Feb 3, 2025

View reviewed changes

forms/tests/test_api.py Outdated Show resolved Hide resolved

henrinie-nc reviewed Feb 3, 2025

View reviewed changes

forms/tests/test_api.py Outdated Show resolved Hide resolved

henrinie-nc reviewed Feb 3, 2025

View reviewed changes

NC-jsAhonen requested a review from henrinie-nc February 3, 2025 13:16

henrinie-nc reviewed Feb 5, 2025

View reviewed changes

plotsearch/tests/serializers/test_plot_search_serializers.py Outdated Show resolved Hide resolved

henrinie-nc reviewed Feb 5, 2025

View reviewed changes

juho-kettunen-nc requested changes Feb 5, 2025

View reviewed changes

NC-jsAhonen force-pushed the MVJ-112-api-does-not-give-file-url-in-response branch from 4552312 to a2643e3 Compare February 6, 2025 11:41

juho-kettunen-nc approved these changes Feb 7, 2025

View reviewed changes

NC-jsAhonen added 3 commits February 7, 2025 14:57

remove unnecessary and sensitive data from attachment public endpoint…

d22333d

… responses

disable downloading in public attachment viewsets

7c1e9a3

test_attachment_upload: remove docstring test forms attachment upload and download: rename functions and improve comments excluded attachment fields: improve commenting use pytest raises improve assert test serializers

AttachmentPublicViewset: add method not allowed to DELETE

98432fd

NC-jsAhonen force-pushed the MVJ-112-api-does-not-give-file-url-in-response branch 3 times, most recently from 1d75dba to 6004e28 Compare February 7, 2025 13:51

attachments: improve tests

4b646a7

NC-jsAhonen force-pushed the MVJ-112-api-does-not-give-file-url-in-response branch from 6004e28 to 4b646a7 Compare February 7, 2025 14:23

NC-jsAhonen merged commit 2783b9e into develop Feb 7, 2025
2 checks passed

NC-jsAhonen deleted the MVJ-112-api-does-not-give-file-url-in-response branch February 7, 2025 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MVJ-112: area search attachments: hide user and attachment path from API response in public #819

MVJ-112: area search attachments: hide user and attachment path from API response in public #819

NC-jsAhonen commented Jan 24, 2025 •

edited

Loading

henrinie-nc left a comment

NC-jsAhonen commented Jan 27, 2025

henrinie-nc commented Jan 27, 2025 •

edited

Loading

henrinie-nc left a comment

NC-jsAhonen commented Jan 28, 2025

henrinie-nc Jan 29, 2025

henrinie-nc Jan 29, 2025

henrinie-nc Jan 29, 2025

henrinie-nc Feb 3, 2025

henrinie-nc Feb 3, 2025

henrinie-nc commented Feb 3, 2025

henrinie-nc Feb 5, 2025

juho-kettunen-nc Feb 5, 2025

henrinie-nc Feb 7, 2025

juho-kettunen-nc left a comment

juho-kettunen-nc Feb 5, 2025

juho-kettunen-nc left a comment



		@pytest.mark.django_db
		def test_area_search_attachment_post_public(

MVJ-112: area search attachments: hide user and attachment path from API response in public #819

MVJ-112: area search attachments: hide user and attachment path from API response in public #819

Conversation

NC-jsAhonen commented Jan 24, 2025 • edited Loading

henrinie-nc left a comment

Choose a reason for hiding this comment

NC-jsAhonen commented Jan 27, 2025

henrinie-nc commented Jan 27, 2025 • edited Loading

henrinie-nc left a comment

Choose a reason for hiding this comment

NC-jsAhonen commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrinie-nc commented Feb 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juho-kettunen-nc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juho-kettunen-nc left a comment

Choose a reason for hiding this comment

NC-jsAhonen commented Jan 24, 2025 •

edited

Loading

henrinie-nc commented Jan 27, 2025 •

edited

Loading